Skip to content

Extend modeling of linear incidences with unknown coefficients #41

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
May 30, 2025

Conversation

serenity4
Copy link
Member

@serenity4 serenity4 commented May 21, 2025

Implements the Incidence linearity extension outlined in #30.

This extends the two-level linearity information (linear with a known constant factor, and nonlinear) with three more, resulting in:

  • Linear, coefficient is a known constant (Float64 value).
  • Linear, coefficient is an unknown constant (linear).
  • Linear, coefficient is unknown with a known state or time dependence (linear_state_dependent, linear_time_dependent).
  • Linear, coefficient is unknown with both state and time dependence (linear_time_and_state_dependent).
  • Nonlinear (nonlinear - always taints state and time dependence currently).

At the moment, incidence IPO is not capable of distributing constant terms such as (1.0 + u1) * (2.0 + u2), which will result in an incidence with an unknown term represented with a typ of Float64 instead of Const(2.0) as one would expect.

Another limitation is that state dependence to an unspecified variable may prevent us in certain cases from inferring which products (mul_float) or substitutions (apply_linear_incidence) remain linear through multiplication with another variable that exhibits a state dependence. See the _muladd2 test which illustrates this in an IPO context. This may also taint time dependence in IPO when time is provided as argument to a callee that then treats it as a state variable.

Perhaps this is acceptable, or perhaps that justifies using a (sparse) incidence matrix. @Keno feel free to suggest alternate designs if you feel that this one does not satisfy our needs.

To do:

  • Update/add documentation for Incidence/Linearity.
  • Address remaining # XXX comments.
  • Fix or change tests marked as broken (requires distributing constant terms across IPO).
  • Fix thermal fluid benchmark.
  • Add @Incidence macro.
  • Add more tests for nonlinear incidences.
  • Add tests for incidences involving time differentials.

@Keno
Copy link
Member

Keno commented May 21, 2025

At the moment, incidence IPO is not capable of distributing constant terms such as (1.0 + u1) * (2.0 + u2), which will result in an incidence with an unknown term represented with a typ of Float64 instead of Const(2.0) as one would expect.

The intrinsic tfunc can distribute this, right? It's just the IPO representation that can't.

@serenity4
Copy link
Member Author

The intrinsic tfunc can distribute this, right? It's just the IPO representation that can't.

That's correct, the tfunc does it just fine.

incidence = @incidence t *ᵢ u₁
@test incidence.typ === Const(0.0)
@test dependencies(incidence.row) == [1 => linear_state_dependent, 2 => linear_time_dependent]
@test repr(incidence) == "Incidence(f(∝t, ∝u₁))"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a tricky one, I think this one should probably just be f₁(t) * u₁

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it should be f₁(t) * u₁ + f₂(u₁) * t to cover, e.g., @incidence t *ᵢ u₁ + t + u₁ which has the same incidence

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have enough precision to resolve f₂(u₁).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this special case we could resolve it (there is only one state, so state dependence means we depend on that single state), but this would break down as soon as we introduce more states. We'd need to track the full matrix to do that reliably.

Copy link
Member

@topolarity topolarity May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sorry the shorthand was a bit over-precise - the main suggestion is not to break symmetry here in the repr

(i.e. Incidence(f(∝t, ∝u₁)) seems better, now that I understand it)

Copy link
Member Author

@serenity4 serenity4 May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that the intent is to print time as a state coefficient, rather than state as a time coefficient. The symmetry would be broken by the fact that state is a somewhat more central as incidence information than time.

That does seem reasonable to me, but I'm also fine with the current printing.

@Keno
Copy link
Member

Keno commented May 21, 2025

That's correct, the tfunc does it just fine.

Yeah, that's pretty much what I expected. There's various things we could do eventually, but it's fine for now.

incidence = @incidence t *ᵢ u₁
@test incidence.typ === Const(0.0)
@test dependencies(incidence.row) == [1 => linear_state_dependent, 2 => linear_time_dependent]
@test repr(incidence) == "Incidence(f(∝t, ∝u₁))"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it should be f₁(t) * u₁ + f₂(u₁) * t to cover, e.g., @incidence t *ᵢ u₁ + t + u₁ which has the same incidence

@serenity4 serenity4 force-pushed the incidence-lattice branch from 9c41806 to 2f5ced4 Compare May 27, 2025 22:22
@serenity4 serenity4 force-pushed the incidence-lattice branch from aadd570 to e7a97eb Compare May 28, 2025 01:56
@serenity4 serenity4 marked this pull request as ready for review May 29, 2025 20:38
@serenity4
Copy link
Member Author

serenity4 commented May 29, 2025

Tests will need JuliaDiff/Diffractor.jl#304 to get Diffractor to precompile, but they all pass locally for me.

@Keno Keno merged commit ba3368e into JuliaComputing:main May 30, 2025
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants